@AmariahAK: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

🦋 Changeset detected

Latest commit: 4358da5

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

sequenceDiagram
  participant CacheDiff
  participant StoreReader
  participant SelectionSet
  participant DiffResult
  CacheDiff->>StoreReader: request diff
  StoreReader->>SelectionSet: execute selection set
  SelectionSet-->>StoreReader: result and raw missing tree
  StoreReader-->>CacheDiff: complete status
  CacheDiff->>DiffResult: access missing diagnostics
  DiffResult->>StoreReader: construct MissingFieldError lazily

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

hi @apollo-cla i already signed, kindly take a look

Hey @AmariahAK 👋

Appreciate the contribution. A few bits of feedback:

1. I'd like to get some benchmarks on how this change affects the overall performance. Thanks to @wolfie in Incomplete cache diffs eagerly build missing-field diagnostics (JSON.stringify of store objects + MissingFieldError construction) in production #13305, you should be able to use that prewritten benchmark here to help. What I'd like to understand is how each changed piece affects the overall performance (@wolfie did some of this already, but its helpful to see with this actually implemented).
2. I don't think __typename is a sufficient replacement for JSON.stringify because you lose too much information here. Take the following query:

query {
  user {
    id
    name
    bestFriends {
      id
      name
    }
  }
}

where user returns a Person object and bestFriends returns an array of Person objects. JSON.stringify provides enough information to disambiguate between which of the Person objects the missing field applies to. Without it, you're stuck having to use devtools or cache extraction to determine exactly what part of the query is missing data. With a very large query, or with lots of cache written to the data, this can add quite a bit of complexity to debugging. With just __typename, its impossible to tell which of these objects are missing fields.

I'm ok not blindly using JSON.stringify, especially if we have a faster option, but I'd like a solution that doesn't remove so much information (this is why the benchmarks are important so we can figure out how best to balance the two).

For data, we have 1 of 3 "types" of objects that can be included in that message:

• Reference objects (the { __ref: "Person:1" } style objects that point to another cache record)
• Normalized objects (an object which has its keyFields included... typically id)
• Non-normalized objects

For normalized objects, I'd like to experiment with replacing the full stringified object with its cache identity (e.g. cache.identify(normalizedObject)). That should provide enough information to quickly debug while avoiding JSON.stringify.

For non-normalized objects, I think we have to keep JSON.stringify, otherwise debugging becomes very difficult.

For reference objects, I'm ok leaving as-is. JSON.stringify should be cheap since its a single property.

Note these proposed solutions are not exhaustive ideas, but rather the things I could think of that maintains debuggability without the need for JSON.stringify. What we need is to check that against benchmarks to see if we actually see perf improvement or not here (e.g. is JSON.stringify or cache.identify cheaper?) and to weigh that against the current baseline.

I think lazy initializing the error probably helps most of the issue, but again, let's see how much we gain/lose by avoiding JSON.stringify. If its negligible enough, I'd prefer keeping it.

By the way, in the future, its really helpful if you talk through ideas in the issue beforehand rather than pointing your agent at a PR and implementing verbatim against a suggested fix without our input. It reduces churn and helps both us and you avoid unnecessary implementation/review on something that ultimately might not work.

Thanks!

PS - I'd kindly ask that you talk to us as a human for additional communication on this PR rather than just having your agent blindly add commits and respond for you. I'd like your personal feedback on how changes here affect both performance and debuggability because I think that needs some personal taste. That also helps me give you a green light on what works best to solve the underlying issue. Thanks!

Oh I also forgot to mention... I'd love if we can test with #13324 as well. I'm doing some work in that PR (which will be released in 4.3) that currently does more with diff.missing. Its a hot path and might negate some of the work we do here, so I think it would be useful to see how much my PR affects any changes here 🙂

@jerelmiller apologies, for seeing this late, let me do these changes

No problem, thanks!

FYI, I'm working on this branch which is doing a lot of work in readFromStore to better handle incremental responses. There is a good chance both of our branches will conflict. #13324 ended up being a stop-gap solution that only solves part of the issue. I found other cases it doesn't handle and making those changes in the cache ended up being the more optimal approach.

Just wanted to note that since I will likely hold off on merging anything here until I get that work done so that I can test it cleanly with anything in this branch. That also means this change will almost certainly land in 4.3 🙂. Just wanted you to be aware so that you can keep an eye on that branch to possibly test against (though its in a pretty broken state right now 😬)

@jerelmiller i dont think it conflicts, ive made the changes youve requested, could you take a look?

Do you have some benchmarks I can review to understand how the different approaches affect performance? For example, is cache.identify any faster than JSON.stringify, etc?

@jerelmiller
For Benchmarks — I ran a microbenchmark comparing the different message-building approaches on Node.js v25.7.0 (unthrottled). Results in milliseconds per call (100 iterations after warmup):

 Message-building (per missing field):
   JSON.stringify(small normalized, 4 fields)       0.0003 ms
   JSON.stringify(medium embedded, 6 fields)         0.0003 ms  
   JSON.stringify(large embedded, ~8KB)              0.0039 ms   (13x cost)
   __typename lookup only                            0.0000 ms
 
 cache.identify() vs JSON.stringify:
   cache.identify(small normalized)                  0.0008 ms
   cache.identify(medium embedded)                   0.0005 ms
   cache.identify(large embedded)                    0.0005 ms
   JSON.stringify(small normalized)                  0.0002 ms
 
 MissingFieldError:
   new MissingFieldError(...)                        0.0016 ms
 
 Full end-to-end (300 diffs, repo from #13305):
   Total                                             10.2 ms    (0.034 ms/diff)

Takeaways:

1. cache.identify() is slightly slower than JSON.stringify for small normalized objects (0.0008 vs 0.0002 ms). This makes sense — identify has to read __typename, look up type policies, extract key fields — while JSON.stringify of a 4-field object is nearly free. However, both are sub-microsecond and the difference is negligible compared to the MissingFieldError construction cost (0.0016 ms).

2. JSON.stringify cost scales with object size — 0.0003 ms for a small object vs 0.0039 ms for an 8KB object (~13x). This is why the original issue measured real-world impact: with many large embedded parents on the hot diff path, these micro-costs add up.

3. The lazy MissingFieldError construction is the dominant win. At 0.0016 ms per construction, deferring this until diff.missing is actually read saves the most time on the hot path where callers only check diff.complete.

4. The tiered message approach is the right tradeoff. For normalized objects, cache.identify() costs 0.0008 ms (still cheap) but provides disambiguated cache IDs ("Customer:c1 object" vs just "object Customer"). For non-normalized objects, JSON.stringify is the only option that preserves debuggability — and it's only paid when diff.missing is actually read (since the message string is part of the MissingFieldError, which is now lazily constructed).

I can add the benchmark to the pr if youd like

Thats helpful thanks! I'm not as worried about the more expensive cache.identify on smaller objects since those should run fast anyways due to their size. The difference there is likely negligible.

If I'm looking at this correctly, in the worst case (all objects are non-normalized), we would actually have a slight perf regression because now we're paying the cost of cache.identify + JSON.stringify correct?

That said, from a taste perspective, I'd love if you could give me some samples of what the new error message looks like with these scenarios (it would help to log the error.path as well since error.message is just the first string message it can find in path).

• all normalized
• all non-normalized
• mixed normalized + normalized

@jerelmiller
In the worst case (all non-normalized objects) we now pay cache.identify() + JSON.stringify() instead of just JSON.stringify. But cache.identify() for a non-normalized object returns undefined almost instantly (0.0005ms) — it just checks __typename and finds no key fields. The overhead is negligible compared to the JSON.stringify cost itself (0.0039ms for an 8KB object, ~13x the regression), and even more so compared to the old eager MissingFieldError construction (0.0016ms per diff). Since the error is now lazy, the message strings aren't built on the hot path anyway.

For the error message examples:
Re: message samples — here's what each scenario produces:

Scenario 1: All normalized objects

error.message: Can't find field 'name' on Person:1 object

error.path: {
  "user": {
    "name": "Can't find field 'name' on Person:1 object",
    "bestFriends": "Can't find field 'bestFriends' on Person:1 object"
  },
  "otherUser": {
    "name": "Can't find field 'name' on Person:2 object"
  }
}

Each Person is disambiguated by cache ID — Person:1 vs Person:2 makes it immediately clear which part of the query is missing data.

Scenario 2: All non-normalized (keyFields: false)

error.message: Can't find field 'id' on object {
  "__typename": "Person",
  "name": "Alice"
}

error.path: {
  "user": {
    "id": "Can't find field 'id' on object {\n  \"__typename\": \"Person\",\n  \"name\": \"Alice\"\n}",
    ...
  },
  "otherUser": {
    "id": "Can't find field 'id' on object {\n  \"__typename\": \"Person\",\n  \"name\": \"Bob\"\n}",
    ...
  }
}

Full JSON dump — you can see the object's contents to distinguish user (Alice) from otherUser (Bob) even though both are Person.

Scenario 3: Mixed normalized + non-normalized

error.message: Can't find field 'id' on object {
  "bestFriends": [
    { "__typename": "Friend", "name": "Charlie" }
  ]
}

error.path: {
  "user": {
    "id": "Can't find field 'id' on object { ... full user object ... }",
    "name": "Can't find field 'name' on object { ... full user object ... }",
    "bestFriends": {
      "0": {
        "id": "Can't find field 'id' on object {\n  \"__typename\": \"Friend\",\n  \"name\": \"Charlie\"\n}"
      }
    }
  },
  "otherUser": {
    "name": "Can't find field 'name' on Person:{\"id\":\"2\"} object"
  }
}

Three different message formats at play here:

• user is non-normalized (embedded) → full JSON dump with bestFriends array visible
• bestFriends[0] is a non-normalized Friend → full JSON dump
• otherUser is normalized → cache identity Person:{"id":"2"} object

Scenario 4: Reference object

error.message: Can't find field 'missingField' on Person:1 object

error.path: {
  "user": {
    "missingField": "Can't find field 'missingField' on Person:1 object"
  }
}

References unchanged — still Person:1 object (cheap, unique).

Ok awesome, I think that maintains enough information and am happy with that.

@wolfie would you mind looking at the comment above to make sure it retains enough information?

@AmariahAK let me noodle on whether to include these changes in the upcoming 4.3 release, or as a patch in 4.2.x. I'll talk with the team to see if the change in the missing message is enough to count for a minor release or not. I should have an answer for you soon and will do a full review of the code at that time. Thanks!

@jerelmiller
Sounds great, Thanks for the thoughtful feedback throughout!

@wolfie would you mind looking at the comment above to make sure it retains enough information?

Sorry, I've had some busy times so I haven't had the chance to put proper effort into this. However, my issue hasn't really been the clarity of the error messages, but rather that with React, a lot of the queries are re-evaluated as partial during re-rendering causing the construction of heavy error messages that are ultimately thrown away and never shown to the user (since once the state is stable, eventually the queries evaluate properly)

So, in short, I'm okay with the error messages as proposed, but I'm not sure I'm a representative opinion, if that makes sense :).

@wolfie no worries! I mostly just wanted to make sure you didn't have any concerns there. Thanks!

@jerelmiller
All set , pushed the fixes. The changeset now pulls the performance context from the original issue (the ~95–140ms main-thread waste, 90ms in the constructor alone, the sibling-watch-dirtying scenario). Also addressed the other feedback: reverted the unnecessary test reformats in diffAgainstStore.ts and policies.ts, dropped the as StoreObject cast, fixed the message format for identified objects, and restructured that lazy-semantics test to use a single toEqual assertion like the rest of the file does. Ready for another look whenever you get to it.

@jerelmiller sorry for the late response, but the undefined comes from custom read functions returning undefined inside arrays. execSubSelectedArrayImpl filters the array with:

array.filter((item) => item === undefined || context.store.canRead(item))

This intentionally preserves undefined items (to keep array indices stable when an entity is evicted). Then in the .map(), undefined isn't null and isn't an array, so when field.selectionSet exists it gets passed directly to executeSelectionSet as objectOrReference. The old message code did JSON.stringify(objectOrReference, null, 2) which stringifies undefined to the JSON value undefined — hence "object undefined".

The existing ducks test in readFromStore.ts exercises this path: the custom read function maps evicted references to undefined, and execSubSelectedArrayImpl propagates them. The "object {}" fallback in the current code handles it correctly, but the deeper question is whether execSubSelectedArrayImpl should be passing undefined through to executeSelectionSet at all, or short-circuiting earlier. Happy to adjust if you have a preference.

@jerelmiller i answered your q here

@jerelmiller i answered your q a while back, are there any more changes youd like me to do, or any other areas youd like to focus on?

Apologies, its been a very busy couple days for me. Hoping to have a small gap today to come back to this!

@jerelmiller no worries take your time

Ok finally got my PR in place and have some time. I will look at this first thing in the morning. Thanks for your patience!

Hey, no worries, take your time, thanks for taking time to go through this in depth.

Thanks for the review and the merge